-
Notifications
You must be signed in to change notification settings - Fork 379
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(_comp_delimited): prepend prefix to all compreply args #913
fix(_comp_delimited): prepend prefix to all compreply args #913
Conversation
Thank you for tracking down the problem.
I guess the original intent is to compactify the candidate list shown when TAB is pressed twice. $ complete -F _echo echo
$ _echo() { local cur prev words cword split comp_args; _comp_initialize -s -- "$@" && _comp_delimited , -W 'hello{1..3}'; }
$ echo x,y,z,he[TAB][TAB]
hello1 hello2 hello3
$ echo x,y,z,he With this PR, it becomes longer as follows. This gets even longer when we already input many names separated by commas. x,y,z,hello1 x,y,z,hello2 x,y,z,hello3
$ echo x,y,z,hello Now I have tested the behavior with Bash. Bash somehow doesn't do anything when $ bind 'set show-all-if-unmodified on'
$ echo x,y,he[TAB]
hello1 hello2 hello3
$ echo x,y,he # The original word is preserved
$ echo x,y,h[TAB]
hello1 hello2 hello3
$ echo hello # The original word is lost So In the past, there was also a problem (reported twice in #544 and #858) that the I conclude that it's useless to try to adjust the candidate list by a kind of hack. As far as Bash doesn't provide an official way to control the candidate list, it is hard to achieve it in a robust way. So I would vote for the change in this PR. |
de9cd55
to
e2b58f6
Compare
Updated with suggested changes. Regarding "&" - not sure if it's interesting, but for me tab-completion just stops working if I add "&" as part of previous delimited argument:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add tests in the subdirectory test/t/unit
with the filename test_unit_delimited.py
? You can reference other files such as test_unit_ip_addresses.py
for how to make tests. For the implementation of a mock completion function, you may look at _echo
in #913 (comment). To run the test, you can use the following command:
$ pytest test/t/unit/test_unit_delimited.py
The command name pytest
might be pytest-3
depending on the installation.
edit: If you want a detailed output, you can use
$ pytest -v test/t/unit/test_unit_delimited.py
Where did you add $ complete -F _echo echo
$ _echo() { local cur prev words cword split comp_args; _comp_initialize -s -- "$@" && _comp_delimited , -W 'hello{1..3}'; }
$ echo "&",h[TAB]
$ echo "",hello <-- when $prefix is not quoted
$ echo "&",hello <-- when $prefix is quoted as "$prefix"
Isn't this because the longest common prefix of |
e2b58f6
to
7d799ed
Compare
Added tests.
It's late here, I'm getting tired :) I forgot to quote the & - I had a setup like this:
and obviously nothing was happening, because I was tring to complete However if I quote the "&" then I see no difference between quoting the prefix and not quoting it:
There is the same result, even with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Looks perfect!
I finally understood what was my problem with "&" - you were talking about bash 5.2 and pathsub_replacement, while I have older bash (5.1) without that option. |
Thank you for your hard work!
As far as I test, the above behavior seems to be different depending on the quoting of
Ah, I see. Sorry for confusing you. |
CI pre-commit has an error. diff --git a/test/t/unit/test_unit_delimited.py b/test/t/unit/test_unit_delimited.py
index 5eb74bdb..249dc1b6 100644
--- a/test/t/unit/test_unit_delimited.py
+++ b/test/t/unit/test_unit_delimited.py
@@ -2,6 +2,7 @@ import pytest
from conftest import assert_bash_exec
+
@pytest.mark.bashcomp(cmd=None)
class TestUnitDelimited:
@pytest.fixture(scope="class") |
7d799ed
to
9314a42
Compare
Tests are failing, I'll look into it tomorrow. |
9314a42
to
75e8913
Compare
Failing tests weren't too difficult, these are all resulting from changes in completion output. However for ssh-keygen and pgrep tests I'm not sure how to actually fix the tests - currently I just removed the outdated asserions, leaving only |
Two tests are still failing, but I don't understand why. They are failing only in one distribution (ubuntu14), one timeout (in seemingly innocious test), and one environment modification (where there shouldn't be any). |
The second error of the environment modification is probably caused by the first error. There are also many errors in
I reran the CI test twice, but the same error happened in both reruns. I also ran the tests locally, and the error was reproduced with bash 4.2. The following is the error message:
The third line must be the cause of the error. Ah, OK. In Bash 4.2, ((${#COMPREPLY[@]})) && COMPREPLY=("${COMPREPLY[@]/#/"$prefix"}") |
Can you add |
81cfc71
to
6c8eb52
Compare
Done. |
Something broke on centos7, but I don't know how to check it :( |
I realized that the double quotations |
CI seems to pass fine now. Thank you! |
Fixes scop#552. Previously prefix was only prepended if COMPREPLY only contained one argument - this commit fixes it so prefix is prepended to all arguments.
d84f1ec
to
e923e7f
Compare
local i | ||
for i in "${!COMPREPLY[@]}"; do | ||
COMPREPLY[i]="$prefix${COMPREPLY[i]}" | ||
done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this certainly seems to work, it's also somewhat unfortunate, because displaying the completions with the prefix in place makes them harder to read, and with show-all-if-ambiguous
on it appears we wouldn't have to do it (I'm biased, I have it on). So I'm contemplating something like:
local i | |
for i in "${!COMPREPLY[@]}"; do | |
COMPREPLY[i]="$prefix${COMPREPLY[i]}" | |
done | |
# If `show-all-if-ambiguous` is off, existing completions preceding the | |
# comma and the comma itself are lost if there are multiple completions | |
# offered for the thing after the prefix. This does not seem to happen | |
# with it on, so avoid repeating the prefix in that case for readability. | |
# https://github.com/scop/bash-completion/pull/532#issuecomment-873675676 | |
if _comp_readline_variable_on show-all-if-ambiguous; then | |
((${#COMPREPLY[@]} == 1)) && COMPREPLY=("$prefix$COMPREPLY") | |
else | |
local i | |
for i in "${!COMPREPLY[@]}"; do | |
COMPREPLY[i]="$prefix${COMPREPLY[i]}" | |
done | |
fi |
I suppose this would require some addressing/complexity in tests, too.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we have discussed in the past in #546, the current test framework totally relies on set show-all-if-ambiguous on
, so I'm afraid there is no way to test the behavior for the case set show-all-if-ambiguous off
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akinomyoga showed that ((${#COMPREPLY[@]} == 1)) && COMPREPLY=("$prefix$COMPREPLY")
isn't guaranteed to work even if show-all-if-unmodified
is on
(#913 (comment)). Copying the snippet here:
$ bind 'set show-all-if-unmodified on'
$ echo x,y,he[TAB]
hello1 hello2 hello3
$ echo x,y,he # The original word is preserved
$ echo x,y,h[TAB]
hello1 hello2 hello3
$ echo hello # The original word is lost
I'm not an expert here, but it feels very brittle to be relying on some obscure undocumented interaction between options, that might break easily down the road (as showcased above).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes. I was also about to write it. I tried the change suggested by @scop with set show-all-if-ambiguous
and confirmed that it still fails. Also, I was experimenting to see if it is possible to detect the insertion by looking at COMP_TYPE
, but there doesn't seem to be a reliable way.
(Experimental code)
local i | |
for i in "${!COMPREPLY[@]}"; do | |
COMPREPLY[i]="$prefix${COMPREPLY[i]}" | |
done | |
if _comp_has_insertion; then | |
local i | |
for i in "${!COMPREPLY[@]}"; do | |
COMPREPLY[i]="$prefix${COMPREPLY[i]}" | |
done | |
fi |
with
# usage: _comp__get_common_prefix words...
# @var[out] ret
_comp__get_common_prefix()
{
ret=${1-}
shift
local v l u m
for v; do
l=0 u=${#ret}+1
while ((l + 1 < u)); do
((m = (l + u) / 2))
if [[ $v == "${ret::m}"* ]]; then
l=$m
else
u=$m
fi
done
ret=${ret::l}
done
}
# Predict whether any string will be possibly inserted in the current command
# line supposed the current COMPREPLY is passed to Bash as is.
# @var[in] COMPREPLY
# @exit 0 if any string is expected to be inserted, or otherwise 1
_comp_has_insertion()
{
local ret
if ((COMP_TYPE == 9)); then
# TAB: When TAB is pressed (without show-all-if-ambiguous), an
# insertion happens when there is a non-empty common prefix.
_comp__get_common_prefix "${COMPREPLY[@]}"
[[ $ret ]]
elif ((COMP_TYPE == 33)); then
# TAB w/ show-all-if-ambiguous: When TAB is pressed (with
# show-all-if-ambiguous), an insertion happens when there is a
# non-empty unique completion or the common prefix is at least as long
# as the current word.
_comp__get_common_prefix "${COMPREPLY[@]}"
((${#COMPREPLY[@]} == 1)) && [[ $ret ]] ||
((${#ret} >= ${#COMP_WORDS[COMP_CWORD]}))
elif ((COMP_TYPE == 37 || COMP_TYPE == 42)); then
# menu-complete or menu-complete-backward: An insertion always happens
# as far as there is at least one completion.
((${#COMPREPLY[@]} > 0))
else
false
fi
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Then let's just go with the implementation here that is known to work in all cases, you guys have spent more than enough time here already. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a note in e3840d0 as a reminder to take stuff into account if someone is tempted to revisit this later.
Fixes #552.
Previously prefix was only prepended if COMPREPLY only contained one argument - this commit fixes it so prefix is prepended to all arguments.
The problem existed since 4138714, in the very first version of
_comp_delimited
function. I'm not sure why the original only prepended the prefix back when there was only one completion, so I'm worried I'm missing something.